-
-
Notifications
You must be signed in to change notification settings - Fork 416
Cape Town | 26-ITP-Jan | Pretty Taruvinga | Sprint 2 | Form Controls #1097
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This comment has been minimized.
This comment has been minimized.
6 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
cjyuan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good.
Regarding your question:
How did you decide what makes a “valid” customer name, and what rules did you use to validate it?
I would probably reference the answers gathered from AI and Google search.
If there is a rule, then chances are we can use JavaScript to enforce the rule.
Form-Controls/index.html
Outdated
| <input | ||
| type="radio" | ||
| id="color-blue" | ||
| name="color" | ||
| value="blue" | ||
| /> | ||
| <label for="color-blue">Blue</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note:
We can also wrap the radio button inside the label element as:
<label><input type="radio" name="color" value="blue"> Blue</label>This way, we don't have to introduce id and for attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much Cjyuan for your suggestion. I have wrapped the radio buttons as per your suggestion and i hope i did it right
Form-Controls/index.html
Outdated
| required | ||
| /><br><br> | ||
| <fieldset> | ||
| <legend>Choose a T-shirt Colour:</legend> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wordings used on lines 32 and 58 could be made more consistent.
May I suggest showing your code to an AI tool and ask it to check for consistency and possible improvements?
It can often help us catch errors, improve consistency, and reinforce best practices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for pointing out that. i have checked and improved , i hope now its better.

Learners, PR Template
Self checklist
Changelist
Questions
How did you decide what makes a “valid” customer name, and what rules did you use to validate it?